Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix expected-cfg-checks for logging features #3425

Closed
wants to merge 1 commit into from
Closed

Fix expected-cfg-checks for logging features #3425

wants to merge 1 commit into from

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Nov 26, 2024

Fixes #3424.

Rustc 1.84 (nightly) introduced that the unexpected_cfgs lint will now be reported for external macros (see
rust-lang/rust#132577), leading to some of our builds failing due to our level = "forbid" config of unexpected cfg flags.

Here, we add the logging features to the expected features of the workspace to mitigiate this issue and fix builds.

@tnull
Fix expected-cfg-checks for logging features
52bfe27 
Rustc 1.84 (nightly) introduced that the `unexpected_cfg`s lint will now
be reported for external macros (see
rust-lang/rust#132577), leading to some of our
builds failing due to our `level = "forbid"` config of unexpected cfg
flags.

Here, we add the logging features to the expected features of the
workspace to mitigiate this issue and fix builds.
@tnull tnull mentioned this pull request Nov 26, 2024
Closed
dergoegge
dergoegge approved these changes Nov 26, 2024
View reviewed changes
Copy link

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, thank you!

@TheBlueMatt
Copy link
Collaborator

I think the error is telling us something is actually broken here - we're checking the feature flags in downstream crates that use the logging macros but those feature flags don't exist there, resulting in them never being set. ISTM we should instead make log_given_level different macros entirely based on feature flags at the top-level (ie set on lightning).

@tnull
Copy link
Contributor Author

tnull commented Nov 26, 2024

I think the error is telling us something is actually broken here - we're checking the feature flags in downstream crates that use the logging macros but those feature flags don't exist there, resulting in them never being set. ISTM we should instead make log_given_level different macros entirely based on feature flags at the top-level (ie set on lightning).

Ugh, duh. Good point. I wonder if it would be worth dropping the logging features generally, since they seem to be a continuing source of user error/confusion, and likely very few people want to hard-code a maximum log level at compile time?

@TheBlueMatt
Copy link
Collaborator

I'd be okay with that. Originally was thinking that logging would imply more allocations than it actually does thanks to fmt::Arguments hiding the allocation until we actually log. We could also drop all of them but disabling gossip, since that's the only thing anyone ever actually cares to do.

@tnull
Copy link
Contributor Author

tnull commented Nov 27, 2024

I'd be okay with that. Originally was thinking that logging would imply more allocations than it actually does thanks to fmt::Arguments hiding the allocation until we actually log. We could also drop all of them but disabling gossip, since that's the only thing anyone ever actually cares to do.

Hmm, alright. I wonder if @enigbe would be interested in picking up dropping the max_level_* features as part of her upcoming logging related PR? Otherwise I'm happy to open another PR to make sure it lands in time for the release.

@tnull
Copy link
Contributor Author

tnull commented Nov 27, 2024

Closing this for now.

@tnull tnull closed this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dergoegge dergoegge dergoegge approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fuzz: libFuzzer build failure "unexpected cfg condition value: max_level_*"
3 participants
@tnull @TheBlueMatt @dergoegge